Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-72505] f:validateButton finds selected radio button #8832

Merged
merged 5 commits into from
Jan 26, 2024
Merged

[JENKINS-72505] f:validateButton finds selected radio button #8832

merged 5 commits into from
Jan 26, 2024
+31 −3

Conversation

timja
Copy link
Member

@timja timja commented Jan 4, 2024 •
edited
Loading

See JENKINS-72505.

Previously it just found the last button when doing validation that involved a radio button, it should try and find a selected one instead...

Noticed in jenkinsci/azure-vm-agents-plugin#493

Testing done

Unit test, if you run it on master you get:

   4.505 [id=25]	WARNING	h.i.i.InstallUncaughtExceptionHandler#handleException: Caught unhandled exception with ID 03a79f27-87f9-4111-a5bc-403a8e405e76
org.junit.ComparisonFailure: expected:<[]f> but was:<[reallynot]f>

Manually:

Tested with a validateButton when editing a template in an azure cloud jenkinsci/azure-vm-agents-plugin#493.
Previously the last radio button was being sent to the method handling the validate button, now it's the selected radio.
I also added an extra radio button locally to make sure it wasn't just taking the first one in the dom by continuing all the way.

Proposed changelog entries

  • JENKINS-72505, Find selected radio option when validating instead of the last one

Proposed upgrade guidelines

N/A

Submitter checklist
Beta Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist
Beta Give feedback

@timja timja added web-ui The PR includes WebUI changes which may need special expertise bug For changelog: Minor bug. Will be listed after features labels Jan 4, 2024
@timja timja mentioned this pull request Jan 4, 2024
Merged
@timja timja force-pushed the fix-radio-validation branch from 69785b4 to 2d9a276 Compare January 5, 2024 10:54
@timja timja changed the title JENKINS-TODO f:validateButton finds selected radio button JENKINS-72505 f:validateButton finds selected radio button Jan 5, 2024
@timja timja force-pushed the fix-radio-validation branch from 2d9a276 to e9b7c0b Compare January 5, 2024 20:34
@timja timja marked this pull request as ready for review January 5, 2024 20:37
@timja timja requested a review from a team January 5, 2024 20:38
@NotMyFault NotMyFault changed the title JENKINS-72505 f:validateButton finds selected radio button [JENKINS-72505] f:validateButton finds selected radio button Jan 6, 2024
@NotMyFault NotMyFault requested review from a team January 6, 2024 09:15
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jan 6, 2024
@NotMyFault NotMyFault requested a review from a team January 10, 2024 09:00
@yaroslavafenkin yaroslavafenkin added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jan 11, 2024
@NotMyFault NotMyFault removed the request for review from a team January 12, 2024 10:43
@timja timja mentioned this pull request Jan 13, 2024
Merged
@timja
Copy link
Member Author

timja commented Jan 25, 2024

@jenkinsci/core-pr-reviewers possible to get a review for this simple bug please?

@MarkEWaite MarkEWaite mentioned this pull request Jan 25, 2024
Merged
aneveux
aneveux approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@aneveux aneveux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot for the fix!

test/src/test/java/lib/form/ValidateButtonTest.java Show resolved Hide resolved
MarkEWaite
MarkEWaite approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 25, 2024
@MarkEWaite MarkEWaite merged commit d2a9fd2 into jenkinsci:master Jan 26, 2024
17 checks passed
@timja timja deleted the fix-radio-validation branch January 26, 2024 22:12
NotMyFault pushed a commit to NotMyFault/jenkins that referenced this pull request Feb 8, 2024
@timja @NotMyFault
[JENKINS-72505] f:validateButton finds selected radio button (jenkins…
2f11900 
…ci#8832)

JENKINS-72505 f:validateButton finds selected radio button

Co-authored-by: Mark Waite <[email protected]>
Co-authored-by: Alexander Brandes <[email protected]>
(cherry picked from commit d2a9fd2)
@timja timja mentioned this pull request Jul 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotMyFault NotMyFault NotMyFault approved these changes

@aneveux aneveux aneveux approved these changes

@MarkEWaite MarkEWaite MarkEWaite approved these changes

Assignees
No one assigned
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timja @MarkEWaite @aneveux @NotMyFault @yaroslavafenkin